Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add AllowedHosts to Actor Definitions and Config Database #21363

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Add AllowedHosts to Actor Definitions and Config Database #21363

merged 4 commits into from
Jan 13, 2023

Conversation

evantahler
Copy link
Contributor

@evantahler evantahler commented Jan 13, 2023 •
edited
Loading

Part of the Network Isolation epic. Closes #21183

This PR updates the actor definitions to provide allowedHosts.hosts, which is an array of URLs (or config matchers) which instances of this connector will be allowed to connect to.

There are a few logical cases:

  • If allowedHosts is not present on the actor definition, this connector does not have any network restrictions.
  • If allowedHosts.hosts is present on the actor definition, and is an empty array (see example included for source-faker), this connector is denied all network access.
  • If allowedHosts.hosts is present on the actor definition, and the array has content, access will be granted to those hosts, IPs, or patterns

@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 00:41 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 00:42 — with GitHub Actions Inactive
@evantahler
Copy link
Contributor Author

evantahler commented Jan 13, 2023 •
edited
Loading

I wanted to add some validation that allowedHosts match some known formats (URLs, IPs, etc), but until we know how we will be doing templating to reference config values (e.g. allowedHosts.hosts: ["${#/definitions/config/subdomain}.snowflake.com"]), we should wait on that part of the story.

evantahler
evantahler commented Jan 13, 2023
View reviewed changes
airbyte-config/init/src/main/resources/seed/source_definitions.yaml
Comment on lines +504 to +505
allowedHosts:
hosts: []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example denies source-faker internet access entirely

@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 00:54 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 00:55 — with GitHub Actions Inactive
@evantahler evantahler changed the title Add AllowedHosts to actor_definitions and database Add AllowedHosts to Actor Definitions and Config Database Jan 13, 2023
@evantahler evantahler marked this pull request as ready for review January 13, 2023 00:55
git-phu
git-phu approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@git-phu git-phu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@git-phu
Copy link
Contributor

git-phu commented Jan 13, 2023

Hmm now that I think about it, actor definition makes sense for API connectors, but would we also want to consider supporting allowed hosts for DB connectors?

The existing (non custom) DB connector definitions wouldn't know beforehand what the IP of the target DB is for a specific instance of a connector, but in the context of custom definitions, would we require the DB endpoint to be baked into the definition, or do we have something else in mind here?

@evantahler
Copy link
Contributor Author

evantahler commented Jan 13, 2023 •
edited
Loading

@git-phu yep! The plan is to be able to reference values from the config, e.g allowedHosts.hosts: ["${#/definitions/config/subdomain}.snowflake.com"] of just allowedHosts.hosts: ["${#/definitions/config/host}"]

@git-phu
Copy link
Contributor

git-phu commented Jan 13, 2023

Ah ok I see, thanks!

pedroslopez
pedroslopez approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@pedroslopez pedroslopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/ConfigWriter.java Outdated Show resolved Hide resolved
@evantahler evantahler enabled auto-merge (squash) January 13, 2023 20:00
@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 20:02 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 20:02 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

File Coverage [87.44%] 🍏
V0_40_27_001__AddAllowedHosts.java 100% 🍏
ConfigWriter.java 86.9% 🍏
Total Project Coverage 26.58% 🍏

@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 20:55 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 13, 2023 20:55 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

File Coverage [87.44%] 🍏
V0_40_27_001__AddAllowedHosts.java 100% 🍏
ConfigWriter.java 86.9% 🍏
Total Project Coverage 26.58% 🍏

@evantahler evantahler merged commit f9bef16 into master Jan 13, 2023
@evantahler evantahler deleted the evan/allowed_hosts branch January 13, 2023 21:30
jbfbell pushed a commit that referenced this pull request Jan 13, 2023
@evantahler @jbfbell
Add AllowedHosts to Actor Definitions and Config Database (#21363)
d174dd0 
* Add AllowedHosts to actor_definitions and database

* use objects for better null-ness handling

* Tables.ACTOR_DEFINITIO
This was referenced Jan 19, 2023
Merged
Merged
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Jan 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@git-phu git-phu git-phu approved these changes

@pedroslopez pedroslopez pedroslopez approved these changes

@davinchia davinchia Awaiting requested review from davinchia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Read allowed_hosts from actor definitions yaml and store in database
3 participants
@evantahler @git-phu @pedroslopez